Skip to content

feat(lint): refactor LinterConfig and add 3 new linting rules#12581

Open
milosdjurica wants to merge 29 commits intofoundry-rs:masterfrom
milosdjurica:feat/linting-multi-contract-files
Open

feat(lint): refactor LinterConfig and add 3 new linting rules#12581
milosdjurica wants to merge 29 commits intofoundry-rs:masterfrom
milosdjurica:feat/linting-multi-contract-files

Conversation

@milosdjurica
Copy link
Contributor

@milosdjurica milosdjurica commented Nov 16, 2025 •
edited
Loading

  • Added a new linting rule that checks for multiple contract-like items (contracts, abstract contracts, interfaces and libraries) per .sol file .
  • Refactored LinterConfig
  • Added 2 new linting rules that checks for interface names and interface file names

Motivation

Enforce best practice.

Solution

I have some doubts regarding this, would like to hear your opinion.

  1. Should the rule skip interfaces and libraries and count only contracts?
  2. Should the rule show linting note for every contract-like item in the file, or only one time per file is enough?

My solution count interfaces and libraries too, and it shows linting note only once per file. Let me know if you want something to be changed :)

PR Checklist

  • Added Tests
  • Added Documentation
  • Breaking changes

Keep up the good work guys, cheers!

@0xrusowsky
Copy link
Contributor

0xrusowsky commented Nov 17, 2025 •
edited
Loading

@milosdjurica first of all thanks for the PR and the initiative, we love new contributions to forge's linter!

i think the idea overall makes sense, however imo we can improve it.

  1. i'd like each contract to be flagged, not just the 2nd one
  2. imo it would make sense to add a custom config for this lint, so that users can decide if they want to allow interfaces and/or libs in the same file. To do so, you'd want to modify the LinterConfig to have a dedicated config for this lint. If we do that, i think we then want to create a new intermediate struct that group all lint configs (i.e. the pre-existing mixed_case_exceptions + your newly introduced config)

lmk if this seems reasonable and if u have any doubts regarding impl

@0xrusowsky 0xrusowsky self-assigned this Nov 17, 2025
@0xrusowsky 0xrusowsky added Cmd-forge-lint Command: forge lint T-external labels Nov 17, 2025
@0xrusowsky 0xrusowsky moved this to In Progress in Foundry Nov 17, 2025
@0xrusowsky 0xrusowsky added this to the v1.6.0 milestone Nov 17, 2025
@milosdjurica
Copy link
Contributor Author

milosdjurica commented Nov 17, 2025

@milosdjurica first of all thanks for the PR and the initiative, we love new contributions to forge's linter!

i think the idea overall makes sense, however imo we can improve it.

  1. i'd like each contract to be flagged, not just the 2nd one
  2. imo it would make sense to add a custom config for this lint, so that users can decide if they want to allow interfaces and/or libs in the same file. To do so, you'd want to modify the LinterConfig to have a dedicated config for this lint. If we do that, i think we then want to create a new intermediate struct that group all lint configs (i.e. the pre-existing mixed_case_exceptions + your newly introduced config)

lmk if this seems reasonable and if u have any doubts regarding impl

Sounds great, I like it! Will look to implement it ASAP :)
Thanks for the response!

@milosdjurica milosdjurica changed the title feat(lint): add multi-contract-file rule feat(lint): refactor LintConfig and add 3 new linting rules Nov 24, 2025
@milosdjurica milosdjurica changed the title feat(lint): refactor LintConfig and add 3 new linting rules feat(lint): refactor LinterConfig and add 3 new linting rules Nov 24, 2025
grandizzy
grandizzy previously requested changes Dec 6, 2025
View reviewed changes
Copy link
Collaborator

@grandizzy grandizzy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thank you! pls fix the build_with_invalid_natspec test to get it going (ideally leave it as before to make sure we're catching that scenario)

@milosdjurica
Copy link
Contributor Author

milosdjurica commented Dec 12, 2025

Hi guys, @0xrusowsky @grandizzy - just a quick ping in case you missed the latest changes 👀

0xrusowsky
0xrusowsky previously approved these changes Dec 13, 2025
View reviewed changes
Copy link
Contributor

@0xrusowsky 0xrusowsky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@grandizzy good to merge?

@grandizzy
Copy link
Collaborator

grandizzy commented Dec 13, 2025

@grandizzy good to merge?

Yep, let's send it 👍

@0xrusowsky 0xrusowsky enabled auto-merge December 13, 2025 08:23
@milosdjurica milosdjurica mentioned this pull request Dec 30, 2025
Closed
3 tasks
onbjerg
onbjerg requested changes Feb 4, 2026
View reviewed changes
Copy link
Member

@onbjerg onbjerg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please update the snapshots (sorry about the slow turnaround)

auto-merge was automatically disabled February 14, 2026 11:42

Head branch was pushed to by a user without write access

@milosdjurica
Copy link
Contributor Author

milosdjurica commented Feb 14, 2026

lmk if need anything else

@milosdjurica milosdjurica requested a review from onbjerg February 14, 2026 14:21
@0xrusowsky 0xrusowsky enabled auto-merge (squash) February 27, 2026 06:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@0xrusowsky 0xrusowsky 0xrusowsky approved these changes

@DaniPopes DaniPopes Awaiting requested review from DaniPopes DaniPopes is a code owner

@mattsse mattsse Awaiting requested review from mattsse mattsse is a code owner

@zerosnacks zerosnacks Awaiting requested review from zerosnacks zerosnacks is a code owner

@grandizzy grandizzy Awaiting requested review from grandizzy grandizzy is a code owner

@onbjerg onbjerg Awaiting requested review from onbjerg onbjerg is a code owner

Requested changes must be addressed to merge this pull request.

Assignees

@0xrusowsky 0xrusowsky

Labels

Cmd-forge-lint Command: forge lint T-external

Projects

Foundry
Status: In Progress

Milestone

v1.6.0

Development

Successfully merging this pull request may close these issues.

4 participants

@milosdjurica @0xrusowsky @grandizzy @onbjerg